change: Make jwt-aud config value a regular expression #4397
Conversation
mkleczek
force-pushed
the
aud-regex
branch
2 times, most recently
from
ea4b9f1 to
2895007
Compare
mkleczek
changed the title
mkleczek
force-pushed
the
aud-regex
branch
3 times, most recently
from
43c8f07 to
153080a
Compare
taimoorzaeem
added
the
breaking change
|
@mkleczek Could you add a summary of the PR? From reading the issues I'm not sure what's the final design here. This would make it easier to review. Also, is the breaking change avoidable by changing the default configuration? |
mkleczek
force-pushed
the
aud-regex
branch
2 times, most recently
from
86bfeb5 to
4276f55
Compare
Done.
Yes. I've decided to set default |
mkleczek
force-pushed
the
aud-regex
branch
2 times, most recently
from
7abddd2 to
0e99087
Compare
| + If the ``aud`` key **is not present** or if its value is ``null`` or ``[]``, PostgREST will interpret this token as allowed for all audiences and will complete the request. | ||
|
|
||
| Examples: | ||
| - To make PostgREST accept ``aud`` claim value from a set ``audience1``, ``audience2``, ``otheraudience``, :ref:`jwt-aud` claim should be set to ``audience1|audience2|otheraudience``. |
There was a problem hiding this comment.
Just to wrap my head around this.
By the docs, right now we support:
If the aud value is a JSON array of strings, it will search every element for a match.
But that's just the JWT and not the jwt-aud config.
So with this change now jwt-aud can have a list of audiences (using or expression), and the JWT can too specify a list of audiences (using JSON array).
There was a problem hiding this comment.
Just to wrap my head around this.
By the docs, right now we support:
If the aud value is a JSON array of strings, it will search every element for a match.
But that's just the JWT and not the
jwt-audconfig.
Yes.
(I think this sentence was not changed)
So with this change now
jwt-audcan have a list of audiences (using or expression), and the JWT can too specify a list of audiences (using JSON array).
Yes.
That's what #2099 is about.
|
|
||
| Examples: | ||
| - To make PostgREST accept ``aud`` claim value from a set ``audience1``, ``audience2``, ``otheraudience``, :ref:`jwt-aud` claim should be set to ``audience1|audience2|otheraudience``. | ||
| - To make PostgREST accept ``aud`` claim value matching any ``https`` URI pointing to a host in ``example.com`` domain, :ref:`jwt-aud` claim should be set to ``https://[a-zA-Z0-9_]*\.example\.com``. |
There was a problem hiding this comment.
Is ReDoS a possibility?
What would be the performance impact of this new feature? Does the JWT cache help here?
There was a problem hiding this comment.
Is ReDoS a possibility?
We validate aud only after JWT authentication (ie. we verify JWT signature first). So it is only possible if an attacker can issue tokens.
Short answer: IMHO no
What would be the performance impact of this new feature? Does the JWT cache help here?
Performance impact needs to be verified.
Right now we do not cache claims validation results so JWT won't help.
We can change it but that will require reloading JWT cache not only whenjwt-secret changes but also when jwt-aud is modified. I would postpone this until we are sure regex matching really affects performance.
There is also another potential performance related issue:
60c8a98 introduces syntactic validation of aud claim. Before, as implemented by @taimoorzaeem in #4140, we didn't really check if aud claim is a valid StringOrURI - we only verified that jwt-aud config is syntactically valid. So:
- we did not validate
audclaims syntactically whenjwt-audwas not set - we returned wrong error message when
jwt-audwas set andaudclaim was invalid URI: instead of "aud syntax error" we returned "JWT not in audience" (that's disputable as both are valid rejection reasons) - in case of
jwt-audconfig being a regular expression we cannot really check if it is a valid StringOrURI anymore (the main reason to implement it in this PR)
Checking aud claim syntactically is additional work to perform upon every request so will affect performance (we don't know if noticeably).
I've implemented it in a separate commit so that we can easily get rid of it (as nothing depends on it). Not sure if syntactical check of aud claim is that important anyway. OTOH caching aud claims validation in JWT cache would help with this case as well.
There was a problem hiding this comment.
There is also another potential performance related issue:
60c8a98 introduces syntactic validation of aud claim.
(Note: the above was released on v13.0.4)
@mkleczek Q: The performance hit would only happen if jwt-aud is set right? And with this new PR, jwt-aud will always be set hence the perf impact will happen for every installation (the jwt-aud='.*' regex check will always be done).
Performance impact needs to be verified.
Checking aud claim syntactically is additional work to perform upon every request so will affect performance (we don't know if noticeably).
So to check the above we would need new loadtests with the jwt-aud enabled right?
There was a problem hiding this comment.
There is also another potential performance related issue:
60c8a98 introduces syntactic validation of aud claim.
(Note: the above was released on v13.0.4)@mkleczek Q: The performance hit would only happen if
jwt-audis set right? And with this new PR,jwt-audwill always be set hence the perf impact will happen for every installation (thejwt-aud='.*'regex check will always be done).
No. Currently (ie. in main) we don't check aud claim syntactically at all. We only syntactically check jwt-aud config. But we use equality check to validate aud claim hence we don't accept syntactically invalid auds if jwt-aud is set but accept invalid auds if jwt-aud is not set.
This change was supposed to change that and validate auds syntactically always, before even checking them against jwt-aud config.
Nevertheless, I removed commit introducing this check for now.
Performance impact needs to be verified.
Checking aud claim syntactically is additional work to perform upon every request so will affect performance (we don't know if noticeably).So to check the above we would need new loadtests with the
jwt-audenabled right?
Nope, see above.
mkleczek
force-pushed
the
aud-regex
branch
2 times, most recently
from
1a405a1 to
688cf83
Compare
@mkleczek I think it'd be better to release v14 and leave this one for v16 (shouldn't take long anyway, towards EOY). That way we can see what's the impact of the JWT cache in isolation (since it's enabled by default now) and reduce the amount of breaking changes we have in one release. WDYT? |
I hoped to have #2099 sooner as we have a need for it, but yeah, maybe it's better not to push it. |
mkleczek
force-pushed
the
aud-regex
branch
3 times, most recently
from
ceb0028 to
781d933
Compare
mkleczek
force-pushed
the
aud-regex
branch
13 times, most recently
from
83d3ee8 to
b8bb558
Compare
|
I've reworked this PR and I think it is now in quite a good shape. The prerequisite, the most important and most complex commit is the first one: 0138fdf Second commit is comparatively small change in Config module. Third commit adds I've spend considerable amount of time testing various caching variants (it was quite easy thanks to the changes in the first commit). Decided to change JwtCache (comparing to |
mkleczek
force-pushed
the
aud-regex
branch
2 times, most recently
from
edf86e4 to
893e110
Compare
… decisions JWT cache implementation introduced two new modules: Auth.Jwt and Auth.JwtCache. This refactoring reorganizes code in Auth and the above two modules so that reponsibilities and dependencies are more clear: * parseClaims function was moved from Auth.Jwt back to Auth. Thanks to it Auth.Jwt module became independent from AuthResult data structure and role handling. Its only purpose right now is to parse/verify tokens and validate claims * validateClaims function in Auth.Jwt module was split to separate validateAud and validateTimeClaims functions. This change was necessary to allow Auth.JwtCache module to be the only place to decide what validations are cached. * Introduced type level tagging of claim validation results so that it is possible to statically ensure all required validations were performed (see Auth.JwtCache.parseAndValidateClaims signature) * Made Auth.Jwt module independent from Config module: validateAud no longer takes Config as an argument but a (Text -> Bool) function to validate audience values * Auth.JwtCache module was changed so that it is now possilble to cache claims validation results. Tagged claim validation result types are used to ensure all validations are performed regardless of the decision about what should be cached. * JwtCache datatype in Auth.JwtCache module was renamed to CacheState with JwksNotConfigured, NotCaching and Caching constructors. * Creation of a Sieve cache instance was moved to a CacheVariant typeclass function newCache * NeedsReconfiguration typeclass was introduced to handle differences between different CacheVariants in deciding when cache reset is needed (if aud claim validation results are cached we need to reset cache when jwt-aud changes)
This change adds flexibility to aud claim validation. jwt-aud configuration property can now be specified as a regular expression. For example, it is now possible to * configure multiple acceptable aud values with '|' regex operator, eg: 'audience1|audience2|audience3' * accept any audience from a particular domain, eg: 'https://[a-z0-9]*\.example\.com'
|
@mkleczek Sorry for the late reply. I've just noted that after d71cb81 all JWT loadtests are failing with
As first step, maybe you can move that refactor commit in another PR? It looks quite involved and would like to review it separately. |
Yeah, after some more thinking I'm still not happy with it. Marked this PR as draft and will open a new one for refactoring. |
4 participants
This change adds flexibility to aud claim validation. jwt-aud configuration property can now be specified as a regular expression. For example, it is now possible to
Resolves #2099
Subsequent #4419 changes default value of jwt-aud to address #4134